feat(HMS-1245): UI AWS permission check #292

avitova · 2023-08-01T13:27:56Z

The best way to test this is to insert a new permission and use your local backend.

akhil-jha · 2023-08-01T16:51:15Z

Env Build error. Running test again.
/retest

lzap · 2023-08-02T08:02:14Z

Do you have a screenshot of the error message? :-)

avitova · 2023-08-02T13:05:44Z

After a conversation on Slack, we have decided to change this from a step in the stepper to a warning shown only in case of the error. Let me know what you think.
To reproduce this, you can for example use your sources on stage, as well as the image builder, but run the frontend with your local backend where you change the expectedStatement variable. Make sure to produce an error so that the warning is shown. Let me know what you think of this.

akhil-jha · 2023-08-03T05:27:28Z

/retest

lzap · 2023-08-07T11:34:52Z

@amirfefer have another PR where he shows a warning message in a different format, please talk to each other and pick one style.

Nitpick: join the permissions with , (comma space)

ezr-ondrej

Did we test this with Azure? The request for permissions will probably fail, right, do we handle it gracefully? The code doesn't seem to do this explicitly, so at minimum it reads bad, because we run checkAWSPerms for all providers.

Just a thought (feel free to ignore): did you think about checking the permissions reactively, maybe when we get AWS error. In that way we would save up a request to API and AWS, I do not think it's strictly necessary and this might be a better solution :)

src/Components/ProvisioningWizard/steps/ReservationProgress/index.js

lzap · 2023-08-07T15:26:21Z

did you think about checking the permissions reactively

Yes, for AWS it means parsing string error and trying to figure out what was wrong. Not too clean.

But good point - this must not break Azure/GCP and it should be implemented in a way that we can easily add the same support into Azure/GCP too.

lzap · 2023-08-07T15:26:55Z

because we run checkAWSPerms for all providers.

This is bad, needs to be fixed.

amirfefer · 2023-08-07T15:50:16Z

Thanks @avitova! My concern is it might confuse users. AFAIK, the error doesn't have to be related to the missing permission, the error message is hidden, but the warning is highlighted, advice from @MariSvirik would help :)

src/Components/ProvisioningWizard/steps/ReservationProgress/index.js

lzap · 2023-08-08T12:09:59Z

I take my comments on the warning UI back, @amirfefer explained why it is difficult to implement warnings as we would like to. Let’s focus that on later, for now this is a good enough solution.

Summary from our discussion:

It would be nice to have the check as an extra step before or after Create reservation.
This step would indicate a warning (perhaps a clickable pop over).
Multiple warning steps should be supported ideally for the future.
Significant code changes are required to support that - not subject for this PR.

MariSvirik · 2023-08-09T12:42:06Z

If the permissions are missing launch will fail, correct?
Right now I see a yellow alert, and a red exclamation mark inside the step and a neutral large icon on the top...so I am not sure if this is still in progress or not.

@lzap what would be that extra step? Do you envision it living inside the wizard?

avitova · 2023-08-09T13:37:24Z

@MariSvirik No, they will not. Users could set their permissions according to our recommendations, and in that case, we'd be able to find out missing permissions accurately.
But there are ways in which they would not follow our instructions, and their setup would work anyways. For example, instead of all the needed permissions, they just insert something like "iam:" and "ec2:", which would grant them all currently needed permissions. There are many reasons why it is impossible to detect these rules 100 % accurately.
TLDR; this is just a heads-up for users to check their permissions. I would say this would be helpful when we change the list of expected permissions and need users to update their AWS policies.
So the question is, how should we let users know without confusing them?

lzap · 2023-08-10T08:38:10Z

Now that I think about it, I think the alternative (and maybe better) solution to this is to take this check few screens before when a source is selected. After source is selected, we can trigger an event of requesting source permission check and display a warning there. What you all think? @ezr-ondrej @amirfefer @avitova @MariSvirik

MariSvirik · 2023-08-10T15:57:07Z

@lzap that would be great.
Let's tell the users as soon as possible that there is a problem with permissions.
How long would it take to check?

lzap · 2023-08-11T14:06:15Z

Tens of milliseconds.

avitova · 2023-08-21T15:51:34Z

So I tried to implement what Maria proposed. Let me know what you think about it now.
We need to now the region and source to find out properly what permissions are missing. This should call the permission validation for every change in one of those fields. I thought it might be 1) safer and 2) better for the future to make the helper function more generic for any provider, so that we only return an empty array for other providers. Let me know your thoughts. :)

avitova · 2023-08-22T07:27:01Z

/retest

avitova · 2023-08-22T09:12:30Z

/retest

MariSvirik · 2023-08-22T10:44:47Z

@avitova How can users fix it? We should tell them.
To make it general we can still include an inline alert in this step with an expandable section that would include missing permissions, but that would be too prominent in my opinion.
Is there any other way to find which permissions are missing if we don't name them in the helper text?

I pinged the doc team to check the text.

ragevou · 2023-08-22T11:45:13Z

src/Components/ProvisioningWizard/steps/AccountCustomizations/aws.js

+        {missingPermissions.length != 0 && (
+          <HelperText>
+            <HelperTextItem variant="warning">
+              Warning: Launch may fail, the following permissions are missing: {missingPermissions.join(', ')}


Suggested change

Warning: Launch may fail, the following permissions are missing: {missingPermissions.join(', ')}

Warning: Launch can fail, the following permissions are missing: {missingPermissions.join(', ')}

amirfefer

Thanks, @avitova, looks great!
For marking the select warning validation, you can propagate a validate prop with a warning value to the source select component.

I guess in some cases the list of missing permissions might be long for a helperText, an ExpandedSection like @MariSvirik suggested sounds like a nice solution, also adding some further information on how to add the permissions in the expanded section. You can combine the ExpandedSection with the helperText component by using the toggleContent prop., as you can see in this live demo

src/Components/ProvisioningWizard/steps/ReservationProgress/index.js

src/API/index.js

avitova · 2023-08-23T11:32:30Z

Thank you both @MariSvirik and @amirfefer, for your comments. I have tried to do the permission check more general, even though it is implemented only for AWS for now.
I thought we could also have a ToolTip for missing permissions, but the list could get really long, so maybe it is not the best idea. The expandable section looks like a good idea. I am a bit worried about the scrollbar though. Let me know what you think.

MariSvirik · 2023-08-24T12:48:46Z

@avitova
I talked to other designers and we decided on an inline expandable alert.
Patternfly doesn't use an expandable plain alert component - as displayed on your screenshot. When there is a problem in the field, use this or this for select. (An inline alert may or may not be used there)

my proposal:
the message is general. When expanded, users can see the list of permissions and where to check it. Would it be possible to add a link where they can check those permissions?

avitova · 2023-08-24T14:52:00Z

Nice! We are getting closer and closer. Thank you for your patience. ❤️
We do not have the recommended policies outside our GitHub though. I'm not sure how to refer to our recommended policies in a different way than this. ❤️

avitova · 2023-08-25T12:39:38Z

After conversations with Maria, I have incorporated her input into the changes made.

lzap · 2023-08-28T13:18:56Z

Idea: Implement some limit, if list of missing permission is longer than let’s say 5 you can generate: perm1, perm2, perm3 and 2 more.

amirfefer

Thanks @avitova! It looks great!
I left a few inline comments, mostly nitpicks, and also please verify that the form validation is not broken due to the default addition to the region field.

src/Components/ProvisioningWizard/steps/AccountCustomizations/aws.js

amirfefer · 2023-09-06T09:16:44Z

src/Components/ProvisioningWizard/steps/AccountCustomizations/aws.js

+              <p>
+                Check if <a href="https://console.aws.amazon.com/iam/">policies</a> in your {image.provider} account for the selected region are set
+                as{' '}
+                <a href="https://github.com/RHEnVision/provisioning-backend/blob/main/docs/configure-amazon-role.md#service-account-policy">


I guess it would be better to link to formal red hat docs, not dev docs

I agree. We do not have the list in the RedHat docs, though. 🤔 And it is one more place we'd have to change in case of changed expected policy. We could use this link but that is quite a lot of steps to only access needed permission list.

Could we ... in theory... have the permission list extracted to a file in the backend and load it in code here on the build time - or figure out how to load it to documentation on build time?

We can also play with it as next step tho. I'd either link Sources directly with "Make sure you're following the steps while creating source", or link the official sources documentation with similar message 🤷

Linking to developer docs is not terrible, just not profesional, but given we are explaining the process quite deeply there I'm mostly worried customers might get confused by all the info there :)

I am not sure how we'd want to show the long list here. The link seems like the only sensible option, but I am open if you see a way to show it. We could have one more expandable section with the required permissions if that makes sense.

I was thinking of Popup, but I agree it's not great, let's just link the official doc, for now. Or split up the developer docs to User and Maintainer oriented docs 🤔 but honestly I think the best way is to link the official docs and as follow-up figure out how to pull the information there directly from our repo

Do you suggest we hardcode the list of permissions to the official docs for now? Yeah, we could do that, and add it to the [https://github.com/RHEnVision/provisioning-backend/blob/main/CONTRIBUTING.md#basic-guidelines-for-code-contributions](list of places to change).
At the moment, we need to load the permission list to:

Go file

Javascript file

yaml

markdown

And we can try to eliminate some of these. 🤔

I'm suggesting to split the IAM policy: RHEnVision/provisioning-backend#682 and then include it on build time here.

But I'm more then happy to leave it to a follow-up :)

Contributing guide is perfect first step for sure :)

src/Components/ProvisioningWizard/steps/AccountCustomizations/gcp.js

src/Components/ProvisioningWizard/steps/AccountCustomizations/azure.js

amirfefer · 2023-09-06T09:29:10Z

src/Components/ProvisioningWizard/steps/AccountCustomizations/aws.js

    types: chosenInstanceType ? 'success' : 'default',
    amount: 'success',
+    region: 'default',


Keep in mind that the default validation means that the field is just set and is not validated yet. we use this validations state for the entire form validation, if one field is marked as default or error the form is not validated, and the Next button will be disabled.

ezr-ondrej · 2023-09-13T13:01:10Z

What is the state here? :) it needs rebase now, but apart of that I guess we are almost ready?

avitova force-pushed the AddPermissionCheck branch from 2b59eca to b3e6ce9 Compare August 2, 2023 13:02

ezr-ondrej reviewed Aug 7, 2023

View reviewed changes

src/Components/ProvisioningWizard/steps/ReservationProgress/index.js Outdated Show resolved Hide resolved

lzap reviewed Aug 8, 2023

View reviewed changes

src/Components/ProvisioningWizard/steps/ReservationProgress/index.js Outdated Show resolved Hide resolved

avitova force-pushed the AddPermissionCheck branch from b3e6ce9 to f58cbbd Compare August 21, 2023 15:44

ragevou reviewed Aug 22, 2023

View reviewed changes

amirfefer reviewed Aug 22, 2023

View reviewed changes

src/Components/ProvisioningWizard/steps/ReservationProgress/index.js Outdated Show resolved Hide resolved

src/API/index.js Outdated Show resolved Hide resolved

avitova force-pushed the AddPermissionCheck branch from f58cbbd to 6879fd9 Compare August 23, 2023 11:29

avitova force-pushed the AddPermissionCheck branch 2 times, most recently from f191734 to d610f0b Compare August 24, 2023 14:51

avitova force-pushed the AddPermissionCheck branch from d610f0b to 7e0f63c Compare August 25, 2023 12:38

feat(HMS-1245): UI AWS permission check

a10baad

avitova force-pushed the AddPermissionCheck branch from 7e0f63c to a10baad Compare August 25, 2023 12:40

amirfefer requested changes Sep 6, 2023

View reviewed changes

avitova closed this Oct 9, 2023

	Warning: Launch may fail, the following permissions are missing: {missingPermissions.join(', ')}
	Warning: Launch can fail, the following permissions are missing: {missingPermissions.join(', ')}

feat(HMS-1245): UI AWS permission check #292

feat(HMS-1245): UI AWS permission check #292

Conversation

avitova commented Aug 1, 2023

akhil-jha commented Aug 1, 2023

lzap commented Aug 2, 2023

avitova commented Aug 2, 2023

akhil-jha commented Aug 3, 2023

lzap commented Aug 7, 2023

ezr-ondrej left a comment

Choose a reason for hiding this comment

lzap commented Aug 7, 2023

lzap commented Aug 7, 2023

amirfefer commented Aug 7, 2023 • edited Loading

lzap commented Aug 8, 2023

MariSvirik commented Aug 9, 2023

avitova commented Aug 9, 2023 • edited Loading

lzap commented Aug 10, 2023

MariSvirik commented Aug 10, 2023

lzap commented Aug 11, 2023

avitova commented Aug 21, 2023

avitova commented Aug 22, 2023

avitova commented Aug 22, 2023

MariSvirik commented Aug 22, 2023

Choose a reason for hiding this comment

amirfefer left a comment • edited Loading

Choose a reason for hiding this comment

avitova commented Aug 23, 2023

MariSvirik commented Aug 24, 2023

avitova commented Aug 24, 2023 • edited Loading

avitova commented Aug 25, 2023

lzap commented Aug 28, 2023

amirfefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezr-ondrej Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

ezr-ondrej commented Sep 13, 2023

amirfefer commented Aug 7, 2023 •

edited

Loading

avitova commented Aug 9, 2023 •

edited

Loading

amirfefer left a comment •

edited

Loading

avitova commented Aug 24, 2023 •

edited

Loading

ezr-ondrej Sep 12, 2023 •

edited

Loading

amirfefer Sep 6, 2023 •

edited

Loading